Skip to content

Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated#67621

Open
michaelpri10 wants to merge 8 commits into
apache:mainfrom
michaelpri10:main
Open

Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated#67621
michaelpri10 wants to merge 8 commits into
apache:mainfrom
michaelpri10:main

Conversation

@michaelpri10

@michaelpri10 michaelpri10 commented May 27, 2026

Copy link
Copy Markdown

Make return_immediately default to False and be configurable for all Pub/Sub modules. This field is deprecated and Cloud Pub/Sub documentation warns that this can lead to pull requests returning with 0 messages even when there is a backlog. By default, the return_immediately option should be set to false.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Google Antigravity following the guidelines


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

Important

🛠️ Maintainer triage note for @michaelpri10 · by @potiuk · 2026-07-02 17:46 UTC

Some review feedback from @shahar1 is waiting on you:

  • 1 unresolved review thread(s) from @shahar1 need a reply or a fix.

The ball is in your court — you've been assigned to this PR. Reply or push a fix in each thread, then mark them resolved.

Automated triage — may be imperfect; a maintainer takes the next look.

@michaelpri10
michaelpri10 requested a review from shahar1 as a code owner May 27, 2026 21:48
@boring-cyborg boring-cyborg Bot added area:providers provider:google Google (including GCP) related issues labels May 27, 2026
@boring-cyborg

boring-cyborg Bot commented May 27, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@michaelpri10

Copy link
Copy Markdown
Author

The failing test looks to be unrelated to the changes here and possibly caused by a misconfiguration of GitHub actions (or maybe a transient infrastructure error) due to the error:

Error: Can't find 'action.yml', 'action.yaml' or 'Dockerfile' under '/home/runner/work/airflow/airflow/.github/actions/post_tests_failure'. Did you forget to run actions/checkout before running your local action?
Screenshot 2026-06-03 at 9 33 19 AM

@potiuk

potiuk commented Jun 9, 2026

Copy link
Copy Markdown
Member

@michaelpri10 A few things need addressing before review — see our Pull Request quality criteria.

  • Provider tests. See docs.

No rush.

Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

@michaelpri10

Copy link
Copy Markdown
Author

Hello @shahar1! Gentle ping on the review for this PR, thank you!

@michaelpri10

Copy link
Copy Markdown
Author

@michaelpri10 A few things need addressing before review — see our Pull Request quality criteria.

  • Provider tests. See docs.

No rush.

Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

This change does contain updated provider unit tests to test the relevant changes, which I believe should satisfy the testing criteria.

@shahar1 shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution and apologies for the delay in response!

CC: @VladaZakharova @MaksYermak - I'll appreciate your opinion here, thanks!

Comment thread providers/google/src/airflow/providers/google/cloud/sensors/pubsub.py Outdated
@MaksYermak

Copy link
Copy Markdown
Contributor

Thanks for your contribution and apologies for the delay in response!

CC: @VladaZakharova @MaksYermak - I'll appreciate your opinion here, thanks!

@shahar1 thank you for letting us know!

I agree with you that for this code the default value should change back to True.

About deprecation.
We already had had deprecation warning for this code in the past and return_immediately parameter was removed by this #23231. After that because of issue in PubSubPullSensor it was retuned back by this #41842. The current PR extends the usage for this deprecated parameter in my opinion it does not make sense because it is deprecated and can be removed any time. But I think we need to make investigate what we can use instead of this parameter for being sure that after removing the PubSubPullSensor will work as expect.

@potiuk

potiuk commented Jun 25, 2026

Copy link
Copy Markdown
Member

@michaelpri10 — this PR has 1 unresolved review thread(s) that still look like they need your attention. Once you've addressed them (push changes and/or reply in-thread), please resolve the threads or reply to confirm, and give the reviewer a nudge for another look. Thanks!

See the PR quality criteria.

Automated first-pass triage note drafted by an AI-assisted tool — may get things wrong; once addressed, a real Apache Airflow maintainer takes the next look. (why automated)


Drafted-by: Claude Code (Opus 4.8); reviewed by @potiuk before posting

…ion warnings for the setting to Pub/Sub modules
@michaelpri10 michaelpri10 changed the title Make return_immediately default to False and be configurable for all Pub/Sub modules Make return_immediately configurable for all Pub/Sub modules and mark the setting as deprecated Jul 6, 2026
@michaelpri10

Copy link
Copy Markdown
Author

I've updated the PR to change back to setting return_immediately==True by default with a deprecation notice per this comment. I will note that PubSubHook currently defaults to return_immediately==False, which has been in place since at least #6096 (~7 years ago), so I've left that as is.

With regards to the initial attempt to remove the return_immediately field in #23231 and the subsequent issue in #41838, the cause of that issue was because the field defaulted to True. This specific issue is noted in the Cloud Pub/Sub documentation:

If your client is using unary Pull, check if the returnImmediately field is set to true. This is a deprecated field that tells the Pub/Sub service to respond to the pull request immediately even if it returns with no messages. This can result in pull requests returning with 0 messages even when there is a backlog.

Thus, I think it would be beneficial to eventually switch the default to False (and remove the field completely later on).

@michaelpri10
michaelpri10 requested a review from shahar1 July 6, 2026 17:47
@shahar1

shahar1 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

@michaelpri10 please review current CI failures

Comment thread providers/google/tests/unit/google/cloud/operators/test_pubsub.py
Comment thread providers/google/tests/unit/google/cloud/sensors/test_pubsub.py
…mmediately parameter and add better test coverage for the return_immediately parameter
@michaelpri10

Copy link
Copy Markdown
Author

@michaelpri10 please review current CI failures

@shahar1 I've updated the tests to fix the current CI failures.

@aaron-y-chen

Copy link
Copy Markdown
Contributor

LGTM

@michaelpri10

Copy link
Copy Markdown
Author

@shahar1 Had to make a few more changes to fix some tests, but all checks are passing now!

@shahar1 shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there!

Comment thread providers/google/src/airflow/providers/google/cloud/operators/pubsub.py Outdated
Comment thread providers/google/src/airflow/providers/google/cloud/hooks/pubsub.py Outdated
Comment thread providers/google/tests/unit/google/cloud/hooks/test_pubsub.py Outdated
…ue of return_immediately to False and undo all changes to PubSubHook as it already defaults return_immediately to False
@michaelpri10

Copy link
Copy Markdown
Author

Almost there!

Thank you @shahar1! I've addressed the comments.

@michaelpri10
michaelpri10 requested a review from shahar1 July 24, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants